Skip to content

fix(eff_ob): the packet names an entity and switches an effect on or off - #508

Merged
erwan-joly merged 1 commit into
NosCoreIO:masterfrom
denislauri1999:pr/eff-ob-shape
Aug 31, 2026
Merged

fix(eff_ob): the packet names an entity and switches an effect on or off#508
erwan-joly merged 1 commit into
NosCoreIO:masterfrom
denislauri1999:pr/eff-ob-shape

Conversation

@denislauri1999

Copy link
Copy Markdown
Contributor

The model has ExtraSpace, MapX, MapY, Unknown, EffectId. The client
reads something else.

Its handler takes the first character of field 0 as the entity kind, field 1
as that entity's id, field 2 as a switch, and field 3 as the effect:

A   := StrToIntDef(field[1], -1)        the entity id
c   := field[0][1]                      the kind - first character only
obj := <lookup>(c, A)
B   := StrToIntDef(field[2], 0)         the switch
E   := StrToIntDef(field[3], 0xFFFF)    the effect
if B = 1:  attach E to obj
if B = 0:  detach whatever obj is carrying
otherwise: nothing

No map coordinate is read anywhere in it.

What was tested

In game, with a positive control so the result cannot be a coincidence - effect
4430 is a ring of magenta arcs and it is unmistakable:

sent seen
eff_ob 1 2 1 4430 the ring appears around the player
eff_ob 1 2 0 4430 nothing
eff_ob 79 115 0 4430 nothing
eff_ob -1 -1 0 4430 nothing
eff 1 2 4430 (control, other route) the ring appears

dotnet build: 0 warnings. dotnet test: 127/127. The serializer corpus
snapshot was edited by hand, two lines, so the diff stays two lines.

What this contradicts, and why I still think it is right

SerializeEffObPacketMatchesTrace pinned eff_ob -1 -1 0 4269, and that line
really is in a packet capture - four times. I replaced the assertion rather than
keep it, so this is worth stating plainly.

That capture is of an older protocol. On the modern client the line does
nothing: the double space puts an empty field in front, so everything shifts by
one and field 2 reads -1, which is neither 1 nor 0, so no branch runs. Even
without the shift, -1 names no entity and the lookup fails first. It cannot be
made to draw anything, which is what the fourth row of the table above shows.

If the intent is to keep the model faithful to that capture rather than to the
client that reads it, say so and I will close this.

The modern client's handler reads four fields: the first character of field 0
as the entity kind, field 1 as its id, field 2 as a switch (1 attaches the
effect, 0 detaches it, anything else does nothing), field 3 as the effect. It
never reads a map coordinate.

Verified on screen: `eff_ob 1 2 1 4430` draws the ring of magenta arcs around
the player and `eff_ob 1 2 0 4430` removes it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 50 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b870013f-54a4-427b-9931-96202fed8543

📥 Commits

Reviewing files that changed from the base of the PR and between 5caf2c1 and b17b324.

📒 Files selected for processing (3)
  • src/NosCore.Packets/ServerPackets/Map/EffObPacket.cs
  • test/NosCore.Packets.Tests/SerializerCorpusTests.ExpressionTreeSerializerOutputIsPinnedForEveryPacket.verified.txt
  • test/NosCore.Packets.Tests/SerializerTest.cs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@erwan-joly
erwan-joly merged commit 62773ab into NosCoreIO:master Aug 31, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants